Skip to content

[OAS/config-schema] Implement discriminated union#246216

Closed
jloleysens wants to merge 9 commits intoelastic:mainfrom
jloleysens:oas/discriminated-union
Closed

[OAS/config-schema] Implement discriminated union#246216
jloleysens wants to merge 9 commits intoelastic:mainfrom
jloleysens:oas/discriminated-union

Conversation

@jloleysens
Copy link
Copy Markdown
Contributor

@jloleysens jloleysens commented Dec 12, 2025

Summary

Related #181994

TODO

@jloleysens jloleysens self-assigned this Dec 12, 2025
@jloleysens jloleysens added Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// release_note:skip Skip the PR/issue when compiling release notes backport:skip This PR does not require backporting labels Dec 12, 2025
@elasticmachine
Copy link
Copy Markdown
Contributor

🤖 Jobs for this PR can be triggered through checkboxes. 🚧

ℹ️ To trigger the CI, please tick the checkbox below 👇

  • Click to trigger kibana-pull-request for this PR!
  • Click to trigger kibana-deploy-project-from-pr for this PR!
  • Click to trigger kibana-deploy-cloud-from-pr for this PR!
  • Click to trigger kibana-entity-store-performance-from-pr for this PR!

Copy link
Copy Markdown
Contributor

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the duplicate work here but I took a quick stab at this last Thursday. I was under the impression there was no one else working on it.

That said, I have a few concerns with this approach.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: I also put up a PoC for a discriminated union type here #246095

Comment on lines +236 to +242
function discriminatedOneOf<T extends Array<ObjectType<any>>>(
discriminator: string,
types: T,
options?: DiscriminatedUnionTypeOptions<UnionOfObjectTypes<T>>
): Type<UnionOfObjectTypes<T>> {
return new DiscriminatedUnionType(discriminator, types, options);
}
Copy link
Copy Markdown
Contributor

@nickofthyme nickofthyme Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This typing does not enforce that the schema objects contain the discriminator.

Enforcing this makes the types harder as I could not find a variadic type solution, so I ended up with this.

function oneOfKind<
  Discriminator extends string,
  A extends PropsWithDiscriminator<Discriminator, Props>,
  B extends PropsWithDiscriminator<Discriminator, Props>
>(
  discriminator: Discriminator,
  types: [ObjectType<A>, ObjectType<B>],
  options?: UnionTypeOptions<ObjectResultUnionType<A | B>>
): Type<ObjectResultUnionType<A | B>>;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree that's a nice improvement!

private readonly discriminator?: string;

constructor(discriminator: string, types: RTS, options?: DiscriminatedUnionTypeOptions<T>) {
let schema = internals.alternatives(types.map((type) => type.getSchema())).match('one');
Copy link
Copy Markdown
Contributor

@nickofthyme nickofthyme Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how this is discriminated based on the discriminator key, this is just a stricter union.

This approach requires exactly one of the provided branches to match the value. If zero or more than one match, the validation fails.

Compared to using alternatives.conditional, which evaluates conditions in the switch array, picks the first matching branch based on the discriminator (or otherwise if provided). Only that branch is applied all others are ignored.

See my solution here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach requires exactly one of the provided branches to match the value. If zero or more than one match, the validation fails.

True, this is an exclusive or hidden under a constructor that will ask for a discriminator key - I believe this is essentially what a discriminated union is.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah but it's not splitting the various schemas by the discriminator. The only discrimination you are doing is within the error handling.

Thus why I think this internal schema is no different than the current oneOf except stricter in that it would not allow multiple matches, much like zod.xor.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also from a performance perspective, the internals.alternatives(...).match('one') will not short-circuit when a match is found because it needs to validate exclusivity.

But with alternatives.conditional this is not the case as the exclusivity is implied by the exhaustive nature of the discriminator, or more simply it only ever checks one of the schemas.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, from an internal implementation perspective it could match this behaviour more optimally, but viewed as a black box they'd work the same, but good point regarding performance!

@jloleysens jloleysens changed the title [OAS/Config-schema] Implement discriminated union [OAS/config-schema] Implement discriminated union Jan 6, 2026
@jloleysens
Copy link
Copy Markdown
Contributor Author

@nickofthyme Thanks for leaving your comments and working on this issue! I think it's possible that we may need something of both of our approaches.

My main concerns are:

  • Correctness - I think both of our approaches are pretty solid, 1 nit is .alternatives().conditional can allow multiple schemas to match if you make a typo/copy-pasta containing the same type literal twice
  • Support known use cases, like @nreese's
  • Good error messages
  • Good DX, as you noted good TS support is important!
  • That we are able to introspect these runtime types to extract JSONSchema for the OAS we generate (OAS docs)

Happy to continue iterating on your PR to resolve correctness nit, @nreese 's use case, OAS introspection

@jloleysens
Copy link
Copy Markdown
Contributor Author

Superseded by #246095

@jloleysens jloleysens closed this Jan 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t//

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants